-
Notifications
You must be signed in to change notification settings - Fork 13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixes Map OOM bug #429
Fixes Map OOM bug #429
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #429 +/- ##
==========================================
- Coverage 86.20% 86.19% -0.01%
==========================================
Files 44 44
Lines 19406 19434 +28
Branches 19406 19434 +28
==========================================
+ Hits 16729 16752 +23
- Misses 1212 1217 +5
Partials 1465 1465 ☔ View full report in Codecov by Sentry. |
@@ -56,7 +56,7 @@ pub struct WasmGenerator { | |||
/// The locals for the current function. | |||
pub(crate) bindings: HashMap<String, Vec<LocalId>>, | |||
/// Size of the current function's stack frame. | |||
frame_size: i32, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some methods return u32 like ListTypeData::get_max_len()
. Although, if something can't be negative shouldn't it be unsigned? I can't see that frame_size is going to be -10 or something.
smallest_sequence_length = if smallest_sequence_length == u32::MAX { | ||
0 | ||
} else { | ||
smallest_sequence_length | ||
}; | ||
|
||
generator.frame_size += return_element_size as u32 * smallest_sequence_length; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lines 634 -> 652 look like create_call_stack_local
but with extra steps.
Not sure how I feel about this:
- on the one hand, it seems this is "optimized" so we don't allocate more space than needed
- on the other hand, any change/optimization to
create_call_stack_local
won't be reflected here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've a fix in mind, but another reason to avoid create_call_stack_local
was we don't need to increase stack-pointer here.
fixes #411